Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the vault format implementation to “Vault V4”, including switching core routines/validators to V4 configuration + keystore models and extending credential-modification flows to optionally use both old and new passkeys (to preserve entropy when available).
Changes:
- Switched core create/unlock/recover/modify-credentials routines to V4 keystore/config models and V4 MAC/keystore derivation paths.
- Extended credential modification API/service pipeline to support “old + new passkey” rotation.
- Bumped
LATEST_VERSIONto V4 and updated version/config validator logic accordingly.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Sdk/SecureFolderFS.Sdk/ViewModels/Views/Overlays/CredentialsOverlayViewModel.cs | Splits login vs register key sequences; wires OldPasskey into selection/confirmation flows. |
| src/Sdk/SecureFolderFS.Sdk/ViewModels/Views/Credentials/CredentialsSelectionViewModel.cs | Threads OldPasskey through to confirmation view model. |
| src/Sdk/SecureFolderFS.Sdk/ViewModels/Views/Credentials/CredentialsConfirmationViewModel.cs | Uses old+new passkey rotation when OldPasskey is provided; adjusts removal flow. |
| src/Sdk/SecureFolderFS.Sdk/Services/IVaultManagerService.cs | Adds overload to modify authentication using both old and new passkeys. |
| src/Platforms/SecureFolderFS.UI/ServiceImplementation/VaultManagerService.cs | Implements the new ModifyAuthenticationAsync overload and forwards to ModifyCredentials routine. |
| src/Core/SecureFolderFS.Core/VaultAccess/VaultParser.cs | Updates/clarifies V4 entropy-preserving rotation documentation. |
| src/Core/SecureFolderFS.Core/Validators/VersionValidator.cs | Treats V3 as unsupported when V4 is latest. |
| src/Core/SecureFolderFS.Core/Validators/ConfigurationValidator.cs | Converts validator to V4 config-only MAC verification. |
| src/Core/SecureFolderFS.Core/Routines/Operational/UnlockRoutine.cs | Removes V3/V4 branching; unlock now reads/derives V4 only. |
| src/Core/SecureFolderFS.Core/Routines/Operational/RecoverRoutine.cs | Removes V3/V4 branching; recover now reads/validates V4 only. |
| src/Core/SecureFolderFS.Core/Routines/Operational/ModifyCredentialsRoutine.cs | Implements V4-only credential rotation, including preserve-entropy rotation when old passkey is available. |
| src/Core/SecureFolderFS.Core/Routines/Operational/CreationRoutine.cs | Creates V4 keystore/config only. |
| src/Core/SecureFolderFS.Core/Routines/IModifyCredentialsRoutine.cs | Adds old+new passkey SetCredentials overload to routine interface. |
| src/Core/SecureFolderFS.Core/Models/SecurityWrapper.cs | Switches wrapper to hold V4 configuration data model. |
| src/Core/SecureFolderFS.Core/DataModels/VaultConfigurationDataModel.cs | Minor property initialization change (recycle bin size). |
| src/Core/SecureFolderFS.Core/DataModels/V4VaultKeystoreDataModel.cs | Expands documentation around SoftwareEntropy lifecycle. |
| src/Core/SecureFolderFS.Core/DataModels/V4VaultConfigurationDataModel.cs | Adds/expands XML docs; removes conversion to legacy config model. |
| src/Core/SecureFolderFS.Core/Constants.cs | Bumps LATEST_VERSION from V3 to V4. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (SelectedViewModel as IDisposable)?.Dispose(); | ||
| SelectionViewModel.Dispose(); | ||
| LoginViewModel.Dispose(); | ||
| _loginKeySequence.Dispose(); | ||
| _registerKeySequence.Dispose(); |
There was a problem hiding this comment.
RegisterViewModel is never disposed in the recovery path: SelectionViewModel.RegisterViewModel is never set when e.IsRecovered == true, and CredentialsResetViewModel.Dispose() only detaches an event handler. This can leak the current authentication view model and leave credential state alive longer than intended. Consider explicitly disposing RegisterViewModel in this overlay's Dispose() (or ensure it is always owned/disposed by another view model in both paths).
| // (V1 or Vn) except LATEST_VERSION are not supported | ||
| (V1 or V2) and not LATEST_VERSION => | ||
| (V1 or V2 or V3) and not LATEST_VERSION => | ||
| throw new NotSupportedException($"Vault version {versionDataModel.Version} is not supported.") { Data = { { "Version", versionDataModel.Version } } }, |
There was a problem hiding this comment.
This change makes V3 vaults fail validation (since LATEST_VERSION is now V4). The SDK login flow treats NotSupportedException as “migratable” and will open a migration UI, but the current migrator selection only supports V1→V2 and V2→V3 (no V3→V4), which will lead to a runtime failure when attempting to migrate a V3 vault. Either keep V3 as a supported unlock version for now, or add a V3→V4 migrator and update migrator selection accordingly.
| public const int V3 = 3; | ||
| public const int V4 = 4; | ||
| public const int LATEST_VERSION = V3; | ||
| public const int LATEST_VERSION = V4; |
There was a problem hiding this comment.
The repo has migration tests covering V1→V2 and V2→V3, but this PR advances LATEST_VERSION to V4 without adding a corresponding migration path/tests. Please add automated coverage for the expected behavior with V3 vaults (either successful unlock as supported, or V3→V4 migration) so regressions are caught early.
| foreach (var key in _loginKeySequence.Keys) | ||
| _registerKeySequence.SetOrAdd(0, key); // First-stage lives at index 0 |
There was a problem hiding this comment.
The seeding logic overwrites index 0 for every key in _loginKeySequence.Keys. When the login flow contains multiple factors (e.g., first-stage + second-stage), the final value at index 0 will be the last key (likely the second-stage), not the first-stage key as the comment intends. This will produce an invalid combined passkey when RegisterViewModel later adds the new second-stage key.
| foreach (var key in _loginKeySequence.Keys) | |
| _registerKeySequence.SetOrAdd(0, key); // First-stage lives at index 0 | |
| var firstStageKey = _loginKeySequence.Keys.FirstOrDefault(); | |
| if (firstStageKey is not null) | |
| _registerKeySequence.SetOrAdd(0, firstStageKey); // First-stage lives at index 0 |
No description provided.